Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

theme: Make dark_plus.toml more accurate to VSCode #8719

Closed
wants to merge 4 commits into from

Conversation

saccarosium
Copy link
Contributor

The current implementation of the Dark+ has some inconsistencies with the original. I've made some modification, witch the most importants are:

  1. Statusbar is now gray instead of blue, to make it more readable;
  2. Whitespace are a more subtle color;
  3. Indent Character is now distinguishable from the whitespaces.

C

Current Updated VSCode
Screenshot 2023-11-04 at 21 30 00 Screenshot 2023-11-04 at 21 28 33 Screenshot 2023-11-04 at 21 29 43

CPP

Current Updated VSCode
Screenshot 2023-11-04 at 21 41 31 Screenshot 2023-11-04 at 21 41 19 Screenshot 2023-11-04 at 21 41 14

@saccarosium saccarosium changed the title Make dark_plus.toml more accurate to VSCode theme: Make dark_plus.toml more accurate to VSCode Nov 4, 2023
@kirawi
Copy link
Member

kirawi commented Nov 4, 2023

If I'm reading the changes correctly, it looks like some of the colors are actually inaccurate to the original theme (e.g. diff.delta). All the colors were taken directly from the definition (with some manual calculating of rgba colors). I'm also against changing the statusline color since it the theme is supposed to be a faithful port of the original.

@saccarosium
Copy link
Contributor Author

Hi @kirawi,
You are right! Actually the diffs mismatch slide throw my radar. It is probably because I've crossed referenced also two vim colorschemes: codedark.vim and vscode.nvim.

As for VSCode's source I checked this two: dark+ and vsdark

I'm also against changing the statusline color since it the theme is supposed to be a faithful port of the original.

I think the statusline's color can be discussed, some people could prefer that.

@kirawi
Copy link
Member

kirawi commented Nov 4, 2023

Neither contain the full definition of the theme. What I had to do was open up the command palette and run Generate color theme from current settings. I would suggest retaining the previous colors since they were already checked, unless you can find a discrepancy between them and the official theme. I'm fine with the updated formatting for the most part. Make sure to retain the previous authorship though.

As for the statusline, I am firmly against it. If anyone wants to change it, then they are free to do so locally. However, it wouldn't be Dark+ if the theme deviated from the original theme.

PopupBack = '#2D2D30'
Selection = '#264F78'
Cursor = '#A6A6A6'
WhiteSpace = '#4D4D4D'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be Whitespace set to 3e3e3d

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I prefer to keep the names in terms of colors. That was originally my intention with this theme, but I never found the time to finish that. Trust me, its less of a headache trying to find a semantic name for things.

@kirawi
Copy link
Member

kirawi commented Nov 4, 2023

Inlay hint colors were changed to the colors of Dark+ experimental in #7611, but they are now defined as

"editorInlayHint.background": "#4d4d4d1a",
"editorInlayHint.foreground": "#969696",

Might be good to update that.

@kirawi
Copy link
Member

kirawi commented Nov 5, 2023

I'll get back to you once I've checked everything.

@saccarosium
Copy link
Contributor Author

What I had to do was open up the command palette and run Generate color theme from current settings

Ok, when I get home I will check it myself also.

For the change a think we stick to the original dark+ theme instead of the new dark modern.

@saccarosium
Copy link
Contributor Author

saccarosium commented Nov 5, 2023

I've came back to the original structure and only made some modifications:

  1. Make the built in type blue like in VSCode
  2. Make the whitespace color #3e3e3d
  3. Make distinguishing what split is focused more easy

@kirawi
Copy link
Member

kirawi commented Nov 5, 2023

Sorry, I didn't mean to indicate that all of your changes were unwanted. I liked how you previously formatted it, but it's fine if you don't want to do that work again. I agree that VSCode Dark Modern should be a separate theme and you are free to open up a new PR with the original changes for that. You would have more creative liberty that route since I only maintain Dark+ :P

@saccarosium
Copy link
Contributor Author

Sorry, I didn't mean to indicate that all of your changes were unwanted.

No problem I did realize a lot of the colors I was used to were influenced by the vim colorschemes I previously used. So there was a lot of personal taste I decided to rollback to your version. After all you are the maintainer.

I liked how you previously formatted it, but it's fine if you don't want to do that work again.

Do you mean separated the names with comments and aligned everything?

@kirawi
Copy link
Member

kirawi commented Nov 5, 2023

So there was a lot of personal taste I decided to rollback to your version.

If it is close enough to the Vim version, then we could probably offer a Vim variant of Dark+. We already offer a Zed variant of onelight.

Do you mean separated the names with comments and aligned everything?

Yeah, it looked nice.

@saccarosium
Copy link
Contributor Author

saccarosium commented Nov 5, 2023

Did the formatting.

If it is close enough to the Vim version, then we could probably offer a Vim variant of Dark+. We already offer a Zed variant of onelight.

I was previously using this colorscheme. But I don't know if it different enough to make it a new variant.

@kirawi
Copy link
Member

kirawi commented Nov 6, 2023

I think it would be acceptable, but I'll leave that to the discretion of the maintainers.

@David-Else
Copy link
Contributor

David-Else commented Nov 6, 2023

Please note line 10 and 11, usize has now gone blue, is this a mistake? Please don't make any changes to the inlay hint colours or statusbar, thanks.

This PR

Screenshot from 2023-11-06 22-02-43

How it was

Screenshot from 2023-11-06 21-58-50

mod hero;
mod world;
mod zombie;
use std::io;
use world::GameState;

#[derive(Debug)]
// a position on a grid to be displayed on the terminal
pub struct Point2d {
    pub x: usize,
    pub y: usize,
}

fn main() {
    let screensize = Point2d { x: 32, y: 32 };
    let mut game_state = GameState::new(&screensize);

    game_state.add_hero('h');
    game_state.add_zombies(64, 'z');

    loop {
        game_state.render_screen();
        println!("Press hjkl to move the hero");

        let mut key = String::new();

        io::stdin()
            .read_line(&mut key)
            .expect("Failed to read line");

        game_state.update(key.trim());
    }
}

@saccarosium
Copy link
Contributor Author

Please note line 10 and 11, usize has now gone blue, is this a mistake?

In VSCode are green but it is inconsistent because it uses a regex highlight system. Since we are using TreeSitter, it does what we tell it to do, I've changed how it displays built-in types in blue and the rest of the types in green, witch does the right thing for C and C++.

@kirawi
Copy link
Member

kirawi commented Nov 9, 2023

So, I think reverting that change in highlighting would be the more correct choice. For some reason (maybe historical), primitive types in C++ are not treated like any other type in both VSCode and CLion. In VSCode, they're actually under storage rather than type. I think the more correct change would be to modify the tree-sitter queries to highlight them under an equivalent scope.

@archseer
Copy link
Member

In VSCode, they're actually under storage rather than type. I think the more correct change would be to modify the tree-sitter queries to highlight them under an equivalent scope.

From what I remember I deliberately omitted storage as a toplevel member. There's keyword.storage.type for modifiers but built in types should just be type.builtin.

Highlighting doesn't need to be identical across editors, and given we're using a different engine (tree-sitter vs regex-based textmate grammar) it's unlikely we can get them 1:1 anyway.

@saccarosium
Copy link
Contributor Author

For recap. Do I revert the built-in types to green?

@saccarosium
Copy link
Contributor Author

I've reverted the type.builtin to be blue again.

@David-Else
Copy link
Contributor

Is it possible to add the highlights for debugging that were introduced in #5957?

| Key                               | Notes                                                                                          |
| ---                               | ---                                                                                            |
| `ui.debug.breakpoint`             | Breakpoint indicator, found in the gutter                                                      |
| `ui.debug.active`                 | Indicator for the line at which debugging execution is paused at, found in the gutter          |
| `ui.highlight.frameline`          | Line at which debugging execution is paused at                                                 |

I am not sure exactly what they are, I looked at https://github.com/microsoft/vscode/blob/main/extensions/theme-defaults/themes/dark_plus.json.

image

@kirawi
Copy link
Member

kirawi commented Feb 3, 2024

Hey, sorry for the delay. I'll be looking at this within a few days.

@David-Else
Copy link
Contributor

@kirawi would you consider adding these as a separate pull request before the next release? I am not sure what colors are best.

Key Notes
ui.debug.breakpoint Breakpoint indicator, found in the gutter
ui.debug.active Indicator for the line at which debugging execution is paused at, found in the gutter
ui.highlight.frameline Line at which debugging execution is paused at

also, as soon as #9647 lands, which will be before the next release, we will need a ui.picker.header. It is hard to see the header without some styling, it looks better with just an underline "ui.picker.header" = { modifiers = ["underlined"] }:

image

but even better might be a colored underline something like:

image

@saccarosium This PR needs to be updated to the latest master as there have been changes to the theme since.

@kirawi
Copy link
Member

kirawi commented Apr 11, 2024

Sorry about leaving this unattended for so long. I've checked it over and it looks good, but there are some merge conflicts to address before it can be merged. I'd understand if you're no longer interested in continuing the PR given the time it took to review though.

@kirawi
Copy link
Member

kirawi commented Apr 11, 2024

@David-Else I haven't used dark plus in a year or so. I'll be removing myself from maintainer of the theme after this PR. I think that your suggestions thus far have been consistent with VSCode, so if you're interested in becoming a new maintainer then I'd be fine with that.

@saccarosium
Copy link
Contributor Author

At this point, I think we can close this. I don't really use helix anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants